Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update documentation to reflect that make_qualification_dict takes 3 arguments. #908

Merged
merged 3 commits into from
Oct 25, 2022
Merged

Update documentation to reflect that make_qualification_dict takes 3 arguments. #908

merged 3 commits into from
Oct 25, 2022

Conversation

xksteven
Copy link
Contributor

@xksteven xksteven commented Oct 3, 2022

make_qualification_dict takes in 3 arguments and these examples in the documentation pass in only two.
Running it with only two gives the following error:

line 401, in main
    make_qualification_dict(
TypeError: make_qualification_dict() missing 1 required positional argument: 'value'

I thought it easier to update the documentation then the code to make the 3rd argument default to None.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 3, 2022
@JackUrb
Copy link
Contributor

JackUrb commented Oct 4, 2022

Thanks for noting and resolving this @xksteven!

@pringshia any idea why the doc deploy test step appears to be failing here?

@pringshia
Copy link
Contributor

@JackUrb updating yarn should fix it, you can run:

yarn set version latest

I have this done in a separate WIP branch but it's part of many other changes that are nowhere near ready to be merged

@xksteven
Copy link
Contributor Author

Let me know if there's anything else I should do for the PR.

@pringshia
Copy link
Contributor

Hey @xksteven thanks for your contribution. I just merged in #920 which should fix the broken test in this PR. Could you please pull down the latest main into your repo, and rebase this PR?

@xksteven
Copy link
Contributor Author

@pringshia All the checks successfully pass now.

@pringshia pringshia self-requested a review October 21, 2022 21:07
@pringshia pringshia requested a review from JackUrb October 21, 2022 21:08
@JackUrb JackUrb merged commit a4aa704 into facebookresearch:main Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pringshia pringshia pringshia approved these changes

@JackUrb JackUrb Awaiting requested review from JackUrb

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@xksteven @JackUrb @pringshia @facebook-github-bot